Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Python-3.10 image #337

Closed
wants to merge 25 commits into from
Closed

Update to Python-3.10 image #337

wants to merge 25 commits into from

Conversation

danielhollas
Copy link
Contributor

Testing if we can upgrade to Python 3.10 Jupyter image.

@danielhollas danielhollas force-pushed the python3.10 branch 2 times, most recently from 91c073f to 43a219f Compare November 29, 2022 14:29
@unkcpz
Copy link
Member

unkcpz commented Dec 2, 2022

@danielhollas, I am redirected from the "bump watchdog" issue. I wonder if we target directly to python 3.11?
But we need anyway to have a discussion on the conda v.s pip install first.

@danielhollas
Copy link
Contributor Author

I would love to switch to 3.11, but I haven't found the jupyter base image with 3.11, so I think we need to wait.

I did some basic testing with the 3.10 image and it seems to work so I would go for it.

@unkcpz
Copy link
Member

unkcpz commented Dec 2, 2022

I would love to switch to 3.11, but I haven't found the jupyter base image with 3.11, so I think we need to wait.

Okay, that makes sense. Just pin me to review when this is ready.

@danielhollas
Copy link
Contributor Author

Both AWB and QeApp test suites passed. 🎉

aiidalab/aiidalab-widgets-base#404
aiidalab/aiidalab-qe#330

This is still blocked on the watchdog bump PR, but once that is merged this could be merged as well.

@danielhollas
Copy link
Contributor Author

For some reason, the Python 3.10.8 started failing when installing aiida-core via mamba, complaining about the ipython package. I don't know why, since even before we always downgrade this package to version 7 to match what aiida-core needs, as can be seen from build logs.

In any case, using the 3.10.6 seems to fix this.

@danielhollas danielhollas mentioned this pull request Dec 13, 2022
5 tasks
@unkcpz
Copy link
Member

unkcpz commented Jul 1, 2023

If the tests all pass, it is fine for me to merge this. Let me know if you think this is ready.

@unkcpz unkcpz force-pushed the main branch 4 times, most recently from 486feb1 to 0897d53 Compare July 10, 2023 23:18
@unkcpz
Copy link
Member

unkcpz commented Jul 17, 2023

Seems the problem with the python version from CI for pyYAML, I'd rather pin the python of CI to 3.10 as well.

@danielhollas
Copy link
Contributor Author

@unkcpz it would be great if you could (in a separate PR) pin all the build dependencies in requirements-dev.txt otherwise I am afraid we will be hitting these kinds of issues often. (My guess the problem is not caused by Python 3.11, but I might be mistaken)

@danielhollas
Copy link
Contributor Author

Looks like the particular issue is caused by a new release of Cython which breaks PyYAML builds.
yaml/pyyaml#724
Pinning PyYAML to earlier version seems to do the trick for now...

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just bring the test back then all is fine.

requirements-dev.txt Show resolved Hide resolved
Comment on lines 42 to 44
# - name: Run tests ✅
#run: VERSION=newly-build pytest -s tests/test-common.py tests/test-${{ inputs.image }}.py --variant ${{ inputs.image }}
#shell: bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you comment this out? The time bottleneck was during the upload the images as artifacts, it is an issue from GitHub side.

@@ -22,7 +22,7 @@ RUN if [ "$TARGETARCH" = "arm64" ]; then \
fi

RUN fix-permissions "${CONDA_DIR}"
RUN fix-permissions "/home/${NB_USER}/.aiida"
RUN fix-permissions "/home/${NB_USER}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@danielhollas
Copy link
Contributor Author

Thanks, just bring the test back then all is fine.

Well, not fine. :-D The tests are failing when trying to install aiidalab-qe because pymatgen fails to build. It would be great if you could take a look where the problem is by downloading the image from ghcr.io and trying out manually.

@unkcpz
Copy link
Member

unkcpz commented Jul 18, 2023

install aiidalab-qe because pymatgen fails to build.

Yes, I just encounter it while testing aiidalab/aiidalab-widgets-base#496, and sorry for the delay of that.
Very sad the installation fail again because of pymatgen, not just us complaining about it, which is not bad 😢 ...

@danielhollas
Copy link
Contributor Author

If the tests all pass, it is fine for me to merge this. Let me know if you think this is ready.

Just a note, the change here is not only updating Python to 3.10, the whole Jupyter stack as well so we really need to test very carefully. For example, I just noticed some strange behaviour in StructureViewer, although I need to still confirm where it is coming from. But there is for example this open issue on nglview nglviewer/nglview#1069

@unkcpz If you want to help move this along, it would be great if you could carefully test the QeApp on this image. Among others, please test the Optimade widget and than merge this PR.
CasperWA/voila-optimade-client#474

@unkcpz
Copy link
Member

unkcpz commented Jul 18, 2023

@danielhollas thanks! I agree a careful test is needed. I realize the new Python version actually brings a new list of dependencies if I run such as pip-compile to get a list of dependencies.

Well, not fine. :-D The tests are failing when trying to install aiidalab-qe because pymatgen fails to build.

I fix the build problem of pymatgen, which seems from CPython as well. Can you bring the test back to see if it works fine?

@danielhollas
Copy link
Contributor Author

I fix the build problem of pymatgen, which seems from CPython as well. Can you bring the test back to see if it works fine?

Tests are now passing, thanks! 🎉

@danielhollas
Copy link
Contributor Author

Just a note that a bunch of notebook AWB tests are failing for the latest image from this PR, see aiidalab/aiidalab-widgets-base#404 , so there's definitely something fishy going on. If it turns out to be too difficult to fix, we can try to downgrade the image to earlier version of Python in the 3.10.x series, which have older Jupyter dependencies.

@unkcpz
Copy link
Member

unkcpz commented Dec 14, 2023

Hi @danielhollas, is there anything can be proceeded or need to investigate on this PR or aiidalab/aiidalab-widgets-base#404?

@danielhollas
Copy link
Contributor Author

Superseded by #455

@danielhollas danielhollas deleted the python3.10 branch May 14, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants